Skip to content

*: Fix various clippy warnings#2900

Merged
mxinden merged 3 commits intolibp2p:masterfrom
umgefahren:fix-minor-clippy-issues
Sep 16, 2022
Merged

*: Fix various clippy warnings#2900
mxinden merged 3 commits intolibp2p:masterfrom
umgefahren:fix-minor-clippy-issues

Conversation

@umgefahren
Copy link
Copy Markdown
Contributor

@umgefahren umgefahren commented Sep 13, 2022

Description

I just implemented some changes suggested by clippy.

Links to any relevant issues

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

thomaseizinger
thomaseizinger approved these changes Sep 14, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

We do run clippy in CI but with a reduced set of lints. Can we perhaps narrow that set down, now that we have fixed a couple more?

@thomaseizinger thomaseizinger changed the title fixed some warnings that were suggested by clippy *: Fix various clippy warnings Sep 14, 2022
@umgefahren
Copy link
Copy Markdown
Contributor Author

umgefahren commented Sep 14, 2022

Yes we could do that. I'm wondering why clippy doesn't pick that up now. I think it might be because the actions still use actions-rs which is not really necessary anymore, since Rust comes with the runners now and actions-rs might be outdated.

I could make an additional PR updating the Workflows.

Many projects also include some lints from the restriction group. This could be considered.

@thomaseizinger
Copy link
Copy Markdown
Contributor

thomaseizinger commented Sep 15, 2022

Yes we could do that. I'm wondering why clippy doesn't pick that up now. I think it might be because the actions still use actions-rs which is not really necessary anymore, since Rust comes with the runners now and actions-rs might be outdated.

I could make an additional PR updating the Workflows.

Feel free, I am always in favor of using tooling better.

Many projects also include some lints from the restriction group. This could be considered.

Feel free to open an issue with a list of suggestions so we can discuss it :)

mxinden
mxinden approved these changes Sep 16, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @umgefahren.

I will merge here, assuming that the above follow-ups happen in separate pull requests.

@mxinden
Copy link
Copy Markdown
Member

mxinden commented Sep 16, 2022

Ignoring Testground failures for now. See libp2p/test-plans#41.

@mxinden mxinden merged commit c81b06a into libp2p:master Sep 16, 2022
@umgefahren umgefahren mentioned this pull request Sep 19, 2022
Closed
4 tasks
@umgefahren umgefahren deleted the fix-minor-clippy-issues branch September 19, 2022 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomaseizinger thomaseizinger thomaseizinger approved these changes

@mxinden mxinden mxinden approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@umgefahren @thomaseizinger @mxinden